Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

phoebus-setup-hook: add java heap size and change encoding by default for phoebus #97

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Rider128
Copy link
Contributor

@Rider128 Rider128 commented Jun 4, 2024

No description provided.

@minijackson
Copy link
Collaborator

Can you also rename your commit to phoebus-setup-hook: … ?

@Rider128 Rider128 changed the title setup-hook: add java heap size and change encoding by default for phoebus phoebus-setup-hook: add java heap size and change encoding by default for phoebus Jun 5, 2024
@Rider128 Rider128 requested a review from minijackson November 4, 2024 13:18
Matthieu Daniel-Thomas added 2 commits November 7, 2024 09:15
The wrapper was added for edit JAVA_OPTS in options.
@Rider128 Rider128 force-pushed the default-option-phoebus branch from d4a55fd to 53ae6af Compare November 7, 2024 08:25
Add the possibility to enable phoebus in a nixos configuration
Add the possibility to edit java_opts
nixos/module-list.nix Outdated Show resolved Hide resolved
@Rider128 Rider128 force-pushed the default-option-phoebus branch from 53ae6af to da863ca Compare November 7, 2024 08:30
pkgs/epnix/tools/phoebus/setup-hook/setup-hook.sh Outdated Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client/default.nix Outdated Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client/default.nix Outdated Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client/default.nix Outdated Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client/default.nix Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client-unwrapped/default.nix Outdated Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client-unwrapped/default.nix Outdated Show resolved Hide resolved
nixos/modules/phoebus/client.nix Outdated Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client/default.nix Outdated Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client/default.nix Outdated Show resolved Hide resolved
@Rider128 Rider128 requested a review from minijackson November 8, 2024 16:02
nixos/modules/phoebus/client.nix Outdated Show resolved Hide resolved
nixos/modules/phoebus/client.nix Outdated Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client/default.nix Outdated Show resolved Hide resolved
pkgs/epnix/tools/phoebus/client/default.nix Outdated Show resolved Hide resolved
Copy link
Collaborator

@minijackson minijackson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, the options you added won't be shown in the documentation. You'll need to add a docs/nixos-services/options-reference/phoebus-client.rst file, similar to the other files there.

pkgs/epnix/tools/phoebus/client/default.nix Outdated Show resolved Hide resolved
nixos/modules/phoebus/client.nix Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Rider128 Rider128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, the options you added won't be shown in the documentation. You'll need to add a docs/nixos-services/options-reference/phoebus-client.rst file, similar to the other files there.

I add the file

Rider128

This comment was marked as duplicate.


# This wrapper for the `phoebus-unwrapped` executable sets the `JAVA_OPTS`
makeWrapper "${lib.getExe epnix.phoebus-unwrapped}" "$out/bin/$pname" \
--prefix JAVA_OPTS ":" "${java_opts}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JAVA_OPTS is a variable separated by spaces (instead of :). Found this by looking at the generated wrapper script, and saw that it weirdly split the ${java_opts} by :. I don't know if this could lead to issues, but let's not take that risk.

Suggested change
--prefix JAVA_OPTS ":" "${java_opts}"
--prefix JAVA_OPTS " " "${java_opts}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants